Human uterus #244
Human uterus #244
Conversation
There was a problem hiding this comment.
I've only provided comments here for the apply transformation part.
Following are copied from my Slack messages:
Just looking at human uterus scaffold annotations, questions:
where are left/right round ligament of uterus?
Do you think we need to change the transition from junction to regular tubes more quickly, as we have discussed before?
Can you confirm they're happy with vagina orifice being a 1-D edge, not the surface at the end?
Human is lacking lumen/serosa annotations. I think you need separate groups for inside and outside for the whole uterus at a minimum.
Mouse:
Seems to be lacking a lumen/serosa of body, vagina as all other parts have these groups.
The mouse has a wrinkle on the bottom of the horns, caused by having 2 elements on the little bit of the body continuous with the horns.
Hermite can't handle that difference in element size. I was going to say increase the number of elements along horns / longest segment to a higher number, but the real issue here is why it doesn't use one element for that segment since it only has a junction at one end. Can you have a look at the code and see if it's possible to stop it forcing minimum 2 in this case? I may send some pointers too...
Regarding mouse number of elements noted above, you can change this line of code (at line ~527) to allow 1 element in these cases:
if (elementsCountAlong == 1) and startTubeBifurcationData and endTubeBifurcationData:
# at least 2 segments if bifurcating at both ends
elementsCountAlong = 2
Seems to work, but still need to increase number of elements along longest segment (i.e. horn) to say 12 (see image 1).
But, you can still see it's slightly stretched on the bottom.
Then if you go to if you go to 13 or more (image 2) it gets 2 small elements again! 20+ elements looks better again. I know the reason for this: it calculates the number of elements based on the path at the centre, but doesn't re-estimate this after curving back at the junctions. This example is really indicating the need for that to be fixed -- length should be based on the mean around the whole ring.
I also want to point out that the top of the mouse body dips in unexpectedly and the parameters d2 versions 1 and 2 are at an unexpected angle (image 3).
Human again: it doesn't got well with more elements e.g. 10/8 -> 20/16. In fact it seems to have a problem on the front with the latter configuration.
src/scaffoldmaker/scaffoldpackage.py
Outdated
| @@ -210,24 +210,23 @@ def getTransformationMatrix(self): | |||
| [ 0.0, 0.0, 0.0, 1.0 ] ] | |||
| return None | |||
|
|
|||
| def applyTransformation(self): | |||
| def applyTransformation(self, editCoordinates): | |||
There was a problem hiding this comment.
It's a field so call it editCoordinatesField
Add :param: docs for it
The :return: docs also need to explain that None is returned if the field is invalid. Or you could make that case also return False. Note that a plain return statement actually returns None.
src/scaffoldmaker/scaffoldpackage.py
Outdated
| editCoordinates = fieldmodule.createFieldConcatenate([ editCoordinates ] + [ fieldmodule.createFieldConstant([ 0.0 ]*(3 - componentsCount)) ]) | ||
| newCoordinates = editCoordinates |
There was a problem hiding this comment.
I prefer arguments to not be modified by functions so you can still debug what was passed. Hence:
newCoordinates = targetCoordinates = editCoordinatesField if (componentsCount >= 3) else fieldmodule.createFieldConcatenate...
src/scaffoldmaker/scaffoldpackage.py
Outdated
| @@ -240,13 +239,13 @@ def applyTransformation(self): | |||
| #print("applyTransformation: apply translation", self._translation) | |||
| newCoordinates = newCoordinates + fieldmodule.createFieldConstant(self._translation) | |||
| # be sure to delete temporary fields and fieldassignment to reduce messages | |||
| doApply = newCoordinates is not coordinates | |||
| doApply = newCoordinates is not editCoordinates | |||
There was a problem hiding this comment.
doApply = newCoordinates is not targetCoordinates
src/scaffoldmaker/scaffoldpackage.py
Outdated
| if doApply: | ||
| fieldassignment = coordinates.createFieldassignment(newCoordinates) | ||
| fieldassignment = editCoordinates.createFieldassignment(newCoordinates) |
There was a problem hiding this comment.
editCoordinates -> targetCoordinates
src/scaffoldmaker/scaffoldpackage.py
Outdated
| fieldassignment.assign() | ||
| del fieldassignment | ||
| del newCoordinates | ||
| del coordinates | ||
| del editCoordinates |
There was a problem hiding this comment.
del targetCoordinates
del newCoordinates
Do not del editCoordinatesField
src/scaffoldmaker/scaffoldpackage.py
Outdated
| for editFieldName in ['coordinates', 'inner coordinates']: | ||
| editCoordinates = fieldmodule.findFieldByName(editFieldName) | ||
| if editCoordinates.isValid(): | ||
| if applyTransformation: |
There was a problem hiding this comment.
if applyTransformation: should be the first statement above fieldmodule = ... as no need to recheck for every field.
This application to coordinates and inner coordinates is what ScaffoldCreator needs to do on clicking Done as well.
Blend shared derivatives across network junctions
|
@rchristie - I've merged your PR and also made the same edits in my uterus code. Tests have been updated and passed all tests. As you mentioned, there's a twist at the 6-way point when I choose a low target element length (e.g. 4.0) and I am not fixing that in this PR since CJ is working on that code and it will create many conflicts. All good for you to proceed with your review again. |
Note - this PR only covers the human and mouse uterus scaffold built using the bifurcation utils. I will submit another pull request for rat uterus and material coordinates when they are ready.